Skip to content

WFS 2.5D point source - #117

Merged
fs446 merged 1 commit into
masterfrom
wfs_25d_point
Mar 15, 2019
Merged

WFS 2.5D point source#117
fs446 merged 1 commit into
masterfrom
wfs_25d_point

Conversation

@fs446

@fs446 fs446 commented Mar 11, 2019

Copy link
Copy Markdown
Member

I remember we had this discussion a while ago in the Matlab toolbox. I try it here again :-)
I'd vote to change our default WFS 2.5D point source to that driving function originating from Delft, which is compatible with the recent works of Firtha, i.e. the unified WFS framework.
Our currently used version is -- due to the further approximation -- not easy to handle in terms of

  • consistent and proper amplitude normalization
  • introducing/defining other referencing schemes

The PR is merge prepared in terms of our latest changes w.r.t. driving function and docstring example handling, but not for literature references.

I would not mind to supply both driving functions with a consistent and proper name scheme. I did not come up with a meaningful idea, though.

@fs446
fs446 requested review from hagenw, mgeier and spors March 11, 2019 14:49

@hagenw hagenw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual implementation of the equation looks good, I was not able to spot an error.

As I was no longer involved in actual WFS theory for some time now, I totally trust you with the decision that this is the better default driving function ;)

Comment thread sfs/mono/drivingfunction.py Outdated
Comment thread sfs/mono/drivingfunction.py Outdated
Comment thread sfs/mono/drivingfunction.py Outdated
Comment thread sfs/mono/drivingfunction.py Outdated
Comment thread sfs/mono/drivingfunction.py Outdated
@fs446

fs446 commented Mar 14, 2019

Copy link
Copy Markdown
Member Author

The actual implementation of the equation looks good, I was not able to spot an error.

As I was no longer involved in actual WFS theory for some time now, I totally trust you with the decision that this is the better default driving function ;)

Thanks!
Don't wait for this issue for the next larger release. I'm not sure if/when to handle this within the next days.

@fs446

fs446 commented Mar 14, 2019

Copy link
Copy Markdown
Member Author

I made a new push including a master rebase, so this version is up to date with latest dev.
I've added some more comments on the 3D/2D Rayleigh integral handling to give the reader more hints what's going on. I'd vote to leave the cited literature in the docstring for now to give users/readers the chance for fast access of the change.
I've introduced 'point_25d_legacy' that implements the point_25d of versions <=0.4.0 . This could get another name though. I've thought about something like
def _point_25d_from_3d()
def _point_25d_from_2d() (the legacy)
and having point_25d = _point_25d_from_3d

I also introduced a normalising gain in both the examples (which is open for all other examples as well) to make more clear that legacy is not amplitude correct at ref point, but the new one is.

@fs446

fs446 commented Mar 14, 2019

Copy link
Copy Markdown
Member Author

I've thought about something like
def _point_25d_from_3d()
def _point_25d_from_2d() (the legacy)
and having point_25d = _point_25d_from_3d

I'm not sure how to handle the docstring docu then.

@mgeier

mgeier commented Mar 15, 2019

Copy link
Copy Markdown
Member

I like the name point_25d_legacy().

@fs446
fs446 requested a review from hagenw March 15, 2019 13:44
@mgeier

mgeier commented Mar 15, 2019

Copy link
Copy Markdown
Member

You should define the "legacy" function after the new function.

Comment thread sfs/mono/wfs.py Outdated
Comment thread sfs/mono/wfs.py Outdated
Comment thread sfs/mono/wfs.py Outdated
Comment thread sfs/mono/wfs.py Outdated
Comment thread sfs/mono/wfs.py
Comment thread sfs/mono/wfs.py Outdated
Comment thread sfs/mono/wfs.py Outdated
Comment thread sfs/mono/wfs.py Outdated
Comment thread sfs/mono/wfs.py Outdated
Comment thread sfs/mono/wfs.py
Comment thread sfs/mono/wfs.py Outdated
Comment thread sfs/mono/wfs.py Outdated
Comment thread sfs/mono/wfs.py Outdated
Comment thread sfs/mono/wfs.py Outdated
Comment thread sfs/mono/wfs.py Outdated
@fs446

fs446 commented Mar 15, 2019 via email

Copy link
Copy Markdown
Member Author

Comment thread sfs/mono/wfs.py
@mgeier

mgeier commented Mar 15, 2019

Copy link
Copy Markdown
Member

but not doing the same principle for the referencing part

Indeed.

We could, instead of using BibTeX, manually add plain Sphinx citations (https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#citations) to the docstrings.

Then your concern would be satisfied.

I just don't think it's worth it.

The bibtex entry tag alone does not hold this information, I hope we agree on this.

We do.

I could imagine that parsing a DOI out of docstring and grabbing all information for creating a reference list is something people ask for or even have done.

I don't know if anyone has asked for it, but I'm quite sure nobody has implemented it.

The current framework is very good, but we should always try to improve.

Sure, there is room for improvement in the current situation.
But I think the BibTeX extension we are using is state-of-the-art, I didn't find any better extensions.

-old handling was Spors/Rabenstein, 2D to 2.5D
-new one is Delft, 3D to 2.5D
@fs446

fs446 commented Mar 15, 2019

Copy link
Copy Markdown
Member Author

But I think the BibTeX extension we are using is state-of-the-art, I didn't find any better extensions.

Good job on that. I will remind myself from time to time to have a look on this issue. Can be closed for the moment.

@fs446
fs446 merged commit c74e34a into master Mar 15, 2019
@fs446
fs446 deleted the wfs_25d_point branch March 15, 2019 21:31
@fs446

fs446 commented Mar 15, 2019

Copy link
Copy Markdown
Member Author

thx for your support!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants